-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for markers in description #273
Conversation
thanks @tjwp, i will have time to review this tommorow |
/describe --pr_description.publish_description_as_comment=true |
TitleImplementing Support for Description Markers in PR Agent PR Type:Enhancement PR Description:This PR introduces the ability to use description markers or short-codes in PR descriptions, which can be particularly useful when combined with a PR template on GitHub. The PR also provides the option to make labels optional for the describe command and allows users to exclude the header included in the short-code expansion. These changes are configurable and do not alter the prompting of the LLM model. PR Main Files Walkthrough:
|
@tjwp If i understood correctly, you are implementing here another variant of the 'describe' post-processing output, tailored to you and your team's needs. Your output looks nice, and I think having two flavors of this important feature is reasonable, and the (advanced) user can pick his favorite one. I might copy some of your design choices also to the default option :-) However, I have some concerns\comments:
|
I'll describe more below but I think that the markers + templating provide a more flexible way to think about how the LLM results are presented without users needing write post-processors, only templates. GitHub pull request templates already provide a convenient way to specify those templates per-repo.
Tests would help with that but they still would not account for variability in the response from the LLM. The handling of the keys in the response could be more flexible than the exact matching that I used but my experience in testing with GPT-4 was that the keys were reliable.
Another way of looking at this is that it is defining an intermediate structure that is is then injected into a template to produce the final output. From that perspective maybe additional post processors are not needed. It may be that over time additional markers are added and supported. For example, if there were a
From that perspective, the markers would always be used and the template above would be applied if the current description is empty. I didn't implement it this way to minimize change to the current behavior, but a next step could be to fully convert to a template.
We haven't need to modify the prompts too much but I will keep that in mind. As noted in the documentation, the prompt has to conform to the internal interface that provides the expected keys in the output. In that way, I think it is compatible with the marker-centric approach where keys essentially map to specific markers, with some light formatting e.g. for the walkthrough.
Thanks for fixing that! I've never use the publish as comment option so it wasn't on my radar. |
Addresses #213
This change adds support for short-codes or markers in pull request descriptions. The use of these markers works well in combination with a pull request template on GitHub.
This functionality is added to the existing /describe command, but is configurable. I have not made it the default behavior here (use_description_markers=false) but I have configured this mode to be used by default in my organization.
While I was implementing this, I also made the labels optional for the describe command (defaults to public_labels=true). One of my users asked to exclude the header that I included in the short-code expansion so that is also configurable but enabled by default (include_generated_by_header=true).
There is no change to the prompting of the LLM model in the way that this is implemented. It only applies the existing description results to the pull request in a different way.
Example output for this PR
Summary
🤖 Generated by PR Agent at 746140b
This PR introduces support for description markers in PR Agent. These markers are short-codes that can be used in pull request descriptions, and are particularly useful when combined with a pull request template on GitHub. The PR also makes labels optional for the describe command and allows users to exclude the header included in the short-code expansion. The changes are configurable and do not alter the prompting of the LLM model.
Walkthrough
🤖 Generated by PR Agent at 746140b
pr_agent/tools/pr_description.py
: The main changes in this file include the addition of support for description markers. The run method has been updated to prepare data and types based on the prediction, and to prepare marker replacements if the use_description_markers setting is enabled. The _prepare_prediction method has been updated to return None if use_description_markers is enabled andpr_agent:
is not in the user description. New methods _prepare_data, _prepare_types, and _prepare_marker_replacements have been added to handle the new functionality. The _prepare_pr_answer method has been updated to prepare the PR answer based on the AI prediction data.pr_agent/settings/configuration.toml
: New settings have been added to the pr_description section: publish_labels, use_description_markers, and include_generated_by_header. These settings control whether labels are published, whether description markers are used, and whether a generated by header is included, respectively.